Support scalar tweak to rotate holder funding key during splicing #3624

wpaulino · 2025-02-26T19:01:57Z

We introduce a scalar tweak that can be applied to the base funding key to obtain the channel's funding key used in the 2-of-2 multisig. This is used to derive additional keys from the same secret backing the base funding_pubkey, as we have to rotate keys for each successful splice attempt.

The tweak is computed similar to existing tweaks used in BOLT-3, but rather than using the per_commitment_point, we use the txid of the funding transaction the splice transaction is spending to guarantee uniqueness, and the revocation_basepoint to guarantee only the channel participants can re-derive the new funding key.

tweak = SHA256(splice_parent_funding_txid || revocation_basepoint || base_funding_pubkey)
tweaked_funding_key = base_funding_key + tweak

Depends on #3604.
Fixes #3542.

lightning/src/ln/chan_utils.rs

wpaulino · 2025-02-27T20:27:12Z

Pushed an update addressing both of your comments @TheBlueMatt @jkczyz.

jkczyz

Just some minor comments. Feel free to ignore if you prefer. Otherwise, LGTM.

lightning/src/sign/mod.rs

lightning/src/ln/channel.rs

jkczyz · 2025-02-27T21:17:57Z

lightning/src/sign/mod.rs

+	/// NOTE: The [`ChannelPublicKeys::funding_pubkey`] returned will not necessarily correspond to
+	/// the channel's current holder public key, as it may have rotated due a splice. The value
+	/// should not be relied upon once the channel's initial funding transaction has been created.


I wonder if we should return the ChannelPublicKeys as part of the derive_channel_signer so that we can remove this method. Then we wouldn't need this caveat. Doesn't need to be done in this PR if it's going to be involved, though.

It would be nice to remove the method, but I think the caveat would still be there regardless?

lightning/src/ln/chan_utils.rs

codecov · 2025-02-28T00:05:53Z

Codecov Report

Attention: Patch coverage is 68.51852% with 51 lines in your changes missing coverage. Please review.

Project coverage is 89.16%. Comparing base (36ba27a) to head (08edfa7).

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	60.97%	31 Missing and 1 partial ⚠️
lightning/src/util/ser.rs	0.00%	10 Missing ⚠️
lightning/src/sign/mod.rs	80.95%	6 Missing and 2 partials ⚠️
lightning/src/ln/channel.rs	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3624      +/-   ##
==========================================
- Coverage   89.19%   89.16%   -0.04%     
==========================================
  Files         152      152              
  Lines      118681   118795     +114     
  Branches   118681   118795     +114     
==========================================
+ Hits       105860   105918      +58     
- Misses      10236    10289      +53     
- Partials     2585     2588       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/sign/mod.rs

tankyleo · 2025-02-28T21:44:05Z

lightning/src/ln/chan_utils.rs

+// TODO: Expose a helper on `FundingScope` that calls this.
+pub fn compute_funding_key_tweak(base_funding_pubkey: &PublicKey, revocation_basepoint: &PublicKey, splice_parent_funding_txid: &Txid) -> Scalar {
+	let mut sha = Sha256::engine();
+	sha.input(splice_parent_funding_txid.as_byte_array());
+	sha.input(&revocation_basepoint.serialize());
+	sha.input(&base_funding_pubkey.serialize());
+	Scalar::from_be_bytes(Sha256::from_engine(sha).to_byte_array()).unwrap()
+}


I would be more comfortable with a roundtrip to the signer to ask for a fresh funding key to use for the new funding transaction, rather than FundingScope deriving this new public key. Could we expand the existing ChannelSigner::pubkeys call to take some id / idx of the funding transaction we need keys for? Then write the returned ChannelPublicKeys to ChannelTransactionParameters, similar to how we do it today for the initial funding transaction?

I am not sure that FundingScope should define the derivation scheme to use for new funding keys; the signer should do that.

In PR 3606 I propose that we avoid having channel objects impose a particular derivation scheme for TxCreationKeys on TxBuilder - this comment is in the same direction.

I consider this tweak a bit different than others because it's something we're doing locally, not something that both peers of the channel need to agree upon and therefore needs to be customized. While we could certainly go to the signer to ask for a new funding key, this mostly abstracts that away from them (the less work the signer has to do, the better IMO, especially for those not using InMemorySigner). The signer is still free to choose their derivation scheme of choice based on channel_keys_id, we are just proposing a tweak to be added to the funding key at the last mile.

Could we expand the existing ChannelSigner::pubkeys call to take some id / idx of the funding transaction we need keys for?

This would be nice to get rid of the caveat pointed out in the docs of ChannelSigner::pubkeys, but we'd want to guarantee the signer implementation only tweaks the funding key and not any of the others, as that would break the channel.

not something that both peers of the channel need to agree upon and therefore needs to be customized.

Interesting my thinking was - Spec does not dictate how to derive the new funding key, and the counterparty does not care which derivation we use, so LDK should let people customize this derivation.

the less work the signer has to do, the better IMO, especially for those not using InMemorySigner

Also interesting - I've been under the impression that signer does too little, and channel does too much :) At least too much that is specific to ecdsa signatures for example.

The signer is still free to choose their derivation scheme of choice based on channel_keys_id, we are just proposing a tweak to be added to the funding key at the last mile.

Yes I agree signer already has some choice of the derivation scheme. Though letting people customize the derivation of TxCreationKeys in TxBuilder could also be seen as a last mile operation - and at the moment we'd let people customize that part.

This would be nice to get rid of the caveat pointed out in the docs of ChannelSigner::pubkeys, but we'd want to guarantee the signer implementation only tweaks the funding key and not any of the others, as that would break the channel.

Certainly

I guess one benefit of having the signer derive the new funding key is it allows them to reuse that same funding key somewhere else within the commitment transaction as a spending condition or destination. I explored the changes you proposed in this separate branch: https://github.com/wpaulino/rust-lightning/tree/funding-key-tweak-alt

I appreciate you doing this thank you.

If we go for this direction, I would hope we can maintain the invariant on ChannelTransactionParameters that the holder_pubkeys matches the keys expected for the parent_txid.

So in src/chain/package, we could read the pubkeys directly from channel_parameters instead of doing a call to the signer.

lightning/src/ln/chan_utils.rs

lightning/src/sign/mod.rs

We introduce a scalar tweak that can be applied to the base funding key to obtain the channel's funding key used in the 2-of-2 multisig. This is used to derive additional keys from the same secret backing the base funding_pubkey, as we have to rotate keys for each successful splice attempt. The tweak is computed similar to existing tweaks used in [BOLT-3](https://github.com/lightning/bolts/blob/master/03-transactions.md#key-derivation): 1. We use the txid of the funding transaction the splice transaction is spending instead of the `per_commitment_point` to guarantee uniqueness. 2. We include the private key instead of the public key to guarantee only those with knowledge of it can re-derive the new funding key. tweak = SHA256(splice_parent_funding_txid || base_funding_secret_key) tweaked_funding_key = base_funding_key + tweak While the use of this tweak is not required (signers may choose to compute a tweak of their choice), signers must ensure their tweak guarantees the two properties mentioned above: uniqueness and derivable only by one or both of the channel participants.

wpaulino · 2025-03-03T20:37:47Z

Ended up reworking this a bit thanks to @tankyleo's suggestion to allow the signer to customize the tweak if they wish to.

jkczyz · 2025-03-03T21:10:55Z

lightning/src/sign/mod.rs

+	/// `splice_parent_funding_txid` can be used to compute a tweak that can be applied to the
+	/// funding key to obtain the one committed to in the 2-of-2 multisig script for a channel that
+	/// has been spliced at least once. See [`compute_funding_key_tweak`] for an example and more
+	/// details.


"has been spliced at least once" seems if the splice has already occurred, but IIUC this may be used during the initiation of a splice. The sentence is a bit long, too. I'd recommend breaking it into two or rephrasing using a semicolon.

jkczyz · 2025-03-03T21:12:55Z

lightning/src/ln/channel.rs

@@ -2733,11 +2735,10 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		config: &'a UserConfig,
 		current_chain_height: u32,
 		outbound_scid_alias: u64,
-		temporary_channel_id: Option<ChannelId>,
+		temporary_channel_id_fn: Option<impl Fn(&ChannelPublicKeys) -> ChannelId>,


Isn't clear to me why this is needed for the PR or why it needs to be a Fn.

jkczyz · 2025-03-03T21:13:40Z

lightning/src/sign/mod.rs

+/// While the use of this tweak is not required (signers may choose to compute a tweak of their
+/// choice), signers must ensure their tweak guarantees the two properties mentioned above:
+/// uniqueness and derivable only by one or both of the channel participants.


Seems like this should live on the trait somewhere?

jkczyz · 2025-03-03T21:21:18Z

lightning/src/sign/mod.rs

+		let funding_key = self.funding_key(channel_parameters.splice_parent_funding_txid);
+		let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &funding_key);


nit: For these you can do:

let funding_pubkey = self.funding_key(channel_parameters.splice_parent_funding_txid).public_key(secp_ctx);

or

let funding_key = self.funding_key(channel_parameters.splice_parent_funding_txid); let funding_pubkey = funding_key.public_key(secp_ctx);

jkczyz · 2025-03-03T21:28:54Z

lightning/src/sign/mod.rs

@@ -1061,6 +1118,14 @@ impl InMemorySigner {
 		}
 	}

+	/// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+	/// holder's anchor output in a commitment transaction, if one is present.
+	pub fn funding_key(&self, splice_parent_funding_txid: Option<Txid>) -> SecretKey {


nit: Might be worth passing the entire ChannelTransactionParameters to avoid mistakenly passing None. Your call.

jkczyz · 2025-03-03T21:47:31Z

lightning/src/sign/mod.rs

+	fn pubkeys(
+		&self, splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>,
+	) -> ChannelPublicKeys;


Looks like this only used externally now. Isn't clear to me we should keep this as one method or make two methods -- one for initial funding and one for splicing -- to avoid needing to pass secp_ctx when None. Not sure if it matters all that much, but it would make what's happening at the call sites explicit.

wpaulino added the weekly goal Someone wants to land this this week label Feb 26, 2025

TheBlueMatt reviewed Feb 26, 2025

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

jkczyz reviewed Feb 27, 2025

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the funding-key-tweak branch from fda4235 to def03b5 Compare February 27, 2025 20:25

jkczyz reviewed Feb 27, 2025

View reviewed changes

wpaulino force-pushed the funding-key-tweak branch from def03b5 to 4bf1e8b Compare February 27, 2025 23:56

jkczyz reviewed Feb 28, 2025

View reviewed changes

lightning/src/sign/mod.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the funding-key-tweak branch 2 times, most recently from 66b084a to 08edfa7 Compare February 28, 2025 17:52

tankyleo reviewed Feb 28, 2025

View reviewed changes

TheBlueMatt reviewed Feb 28, 2025

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

lightning/src/sign/mod.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the funding-key-tweak branch from 08edfa7 to 21e3ccc Compare March 3, 2025 20:36

wpaulino requested review from TheBlueMatt and jkczyz March 3, 2025 20:37

jkczyz reviewed Mar 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support scalar tweak to rotate holder funding key during splicing #3624

Support scalar tweak to rotate holder funding key during splicing #3624

wpaulino commented Feb 26, 2025 •

edited

Loading

wpaulino commented Feb 27, 2025

jkczyz left a comment

jkczyz Feb 27, 2025

wpaulino Feb 27, 2025

codecov bot commented Feb 28, 2025 •

edited

Loading

tankyleo Feb 28, 2025

wpaulino Feb 28, 2025

tankyleo Feb 28, 2025

wpaulino Mar 1, 2025

tankyleo Mar 1, 2025

wpaulino commented Mar 3, 2025

jkczyz Mar 3, 2025

jkczyz Mar 3, 2025

jkczyz Mar 3, 2025

jkczyz Mar 3, 2025

jkczyz Mar 3, 2025

jkczyz Mar 3, 2025

		let funding_key = self.funding_key(channel_parameters.splice_parent_funding_txid);
		let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &funding_key);

Support scalar tweak to rotate holder funding key during splicing #3624

Are you sure you want to change the base?

Support scalar tweak to rotate holder funding key during splicing #3624

Conversation

wpaulino commented Feb 26, 2025 • edited Loading

wpaulino commented Feb 27, 2025

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino commented Feb 26, 2025 •

edited

Loading

codecov bot commented Feb 28, 2025 •

edited

Loading